Alternative: restrict Navigation permissions and show UI warning if cannot create #37454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +238 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
getdave
commented
Dec 16, 2021
getdave
commented
Dec 16, 2021
lib/navigation.php
Outdated
| @@ -50,6 +50,9 @@ function gutenberg_register_navigation_post_type() { | |||
| 'editor', | |||
| 'revisions', | |||
| ), | |||
| 'capabilities' => array( | |||
| 'create_posts' => 'edit_theme_options', | |||
There was a problem hiding this comment.
Restrict creation of Nav posts to "admin" role level only. Allows us to display a warning immediately upon insertion of Nav block.
Attempting to modify all the permission (as in WordPress/wordpress-develop#2056) will not work because getEntityRecord will not return the Navigation Post for viewing.
getdave
added
[Block] Navigation
getdave
requested review from
tellthemachines and
TimothyBJacobs
as code owners
|
Even if you can't create / edit navigations, you should be able to select one from dropdown. |
|
This sounds like a good option, and it still leaves the possibility for loosening the restrictions later if the right solution can be found. Agree with @spacedmonkey that it'd be good to support selecting menus still, but I suppose the dropdown would have to exclude classic menus, as they're converted to a Code looks good so far. |
getdave
force-pushed
the
try/alt-nav-block-show-ui-warning-if-cannot-create
branch
from
f0bfb94 to
8013fab
Compare
OK I reinstated that feature and created another Notice which appears only when the user is trying to create and there is not existing Nav entity reference.
Is that because Menus are converted into |
Hopefully we can also hide as many menu creation options as possible.
Yep exactly. |
getdave
added
the
Backport to WP 6.7 Beta/RC
|
I am working on e2e tests now. |
adamziel
reviewed
Dec 17, 2021
adamziel
reviewed
Dec 17, 2021
|
It works well, and it is a good start, thank you for working on that @getdave ! There is a bit of a flickering involved:
See the video below: editing.notices.mp4It isn't necessarily a blocker, as this PR solves an important problem, but it would be great to land a fix sooner than later. Maybe the suspense API support will help here once it lands? There's one other non-blocking rough edge I want to point out: The notice is really easy to miss, and even though it is in place, the sidebar still offers to save the menu. An attempt to save it fails, but
It would be nicer to not display it there in the first place, or make the block immutable (e.g. using a templateLock as @talldan suggested). |
adamziel
approved these changes
Dec 17, 2021
There was a problem hiding this comment.
I pointed out some rough edges in this PR, but I don't think any of them is a blocker. The code looks good, the fix works, the problem is important, so I'm approving ✅ I'll defer to @getdave on the best way to apply, postpone, and track the non-blocking issues here.
This comment has been minimized.
This comment has been minimized.
7 tasks
This comment has been minimized.
This comment has been minimized.
|
@tellthemachines I believe you will be managing the backporting for Beta 4? If so please can you look to merge this PR as it's important to fix a key permission bug with the Nav block. Please note that we'll require a companion Trac ticket to alter the 🙇 Thank you in advance. |
tellthemachines
deleted the
try/alt-nav-block-show-ui-warning-if-cannot-create
branch
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
pushed a commit
that referenced
this pull request
Dec 21, 2021
…annot create (#37454) * Test for publish abtility and display warning * Update creation of Nav posts to require admin perms * Only show menu creation option if user has permission to create * Move permission selectors to hook * Show warning if unable to create Navigation Menus * Copy changes from Core patch See https://github.com/WordPress/wordpress-develop/pull/2056/files * Only show error if create is not allowed * Revert "Copy changes from Core patch" This reverts commit 1872f62a454dc41ce787103e3d1830f7ff00d63c. * Use streamlined permissions Kudos to @spacedmonkey for https://github.com/WordPress/gutenberg/pull/37454\#discussion_r771026149 * Remove inline warning and reenable ability to select existing * Refactor Notices to reusable hook * Add notices for creating Menus * Rename dep for clarity * Hide other creation options * Add e2e test * Hide classic Menus from dropdown See https://github.com/WordPress/gutenberg/pull/37454\#issuecomment-996569572 * Fix up e2e tests following changes from rebase * Update to make component props more agnostic * Make component props less tighly coupled to permissions * Remove unneeded undefined fallback * Refactor hooks * Try switch user to admin * Try switching back to admin after each permissions test * Try targetting test that relies on avoiding a 404 from URL details endpoint
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Dec 21, 2021
… type. Adds `'edit_theme_options'` capabilities top restrict Navigation permission. Partial backport from Gutenberg WordPress/gutenberg#37454. Follow-up to [52069], [52145]. Props spacedmonkey, get_dave, hellofromTonya. See #54487. git-svn-id: https://develop.svn.wordpress.org/trunk@52400 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Dec 21, 2021
… type. Adds `'edit_theme_options'` capabilities top restrict Navigation permission. Partial backport from Gutenberg WordPress/gutenberg#37454. Follow-up to [52069], [52145]. Props spacedmonkey, get_dave, hellofromTonya. See #54487. Built from https://develop.svn.wordpress.org/trunk@52400 git-svn-id: http://core.svn.wordpress.org/trunk@51992 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott
pushed a commit
to gMagicScott/core.wordpress-mirror
that referenced
this pull request
Dec 21, 2021
… type. Adds `'edit_theme_options'` capabilities top restrict Navigation permission. Partial backport from Gutenberg WordPress/gutenberg#37454. Follow-up to [52069], [52145]. Props spacedmonkey, get_dave, hellofromTonya. See #54487. Built from https://develop.svn.wordpress.org/trunk@52400 git-svn-id: https://core.svn.wordpress.org/trunk@51992 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This was referenced Mar 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #37289 we were exploring trying to show a warning if the user cannot "make" new Navigation Menus. However this was proving difficult as whilst even low Role users had permission to
createthey don't have the permission topublish. This makes it impossible to show an upfront warning to the user if they are likely to not have permission to publish because the publish action is specific to an individualwp_navigationPost which is not created when you insert the Nav block.This PR proposes another route. We increase the permission gate for the
createaction on thewp_navigationPost type toedit_theme_options.This will effectively mean that only Admins can "create" Navigation posts whilst all other users can only "read".
This matches nicely with the current
Menussystem in WordPress which is also only accessible to Admin users.There is a precedent in Core for adding custom capabilities to "special" post types. This PR takes the same approach for Navigation posts.
Why not set all permissions to
edit_theme_options?This PR does not use the changes from this Core ticket which seeks to alter the perms on the
wp_navigationpost to match those of the Menu post type. Why? Because by adjusting the permissions in this way the post types endpoint will not show thewp_navigationpost and thus it will not be loaded as an entity in Gutenberg. This will mean the check for the entity will fail ingetEntityRecord()before the API request is fired. Therefore we will not be able to "view" the Navigation post.Having spoken with release leads, as requested I have raised this Trac ticket.
As this PR modifies
navigation.phpwe will need a corresponding Core patch. The best candidate is probably the one @spacedmonkey already has open in WordPress/wordpress-develop#2056.Closes #36286
How has this been tested?
First check you cannot create Navigations as a low permission user:
Createa Navigation via buttons in the placeholder.Now check you can still view existing Navigations:
Create Menuoption in theSelect Menudropdown.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).